Skip to content

Conversation

@yaira2
Copy link
Member

@yaira2 yaira2 commented May 22, 2024

Resolved / Related Issues

To prevent extra work, all changes to the Files codebase must link to an approved issue marked as Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.

Steps used to test these changes

Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.

  1. Enter git clone https://github.com/files-community/Files into address bar
  2. Confirm the repo is cloned correctly

@yaira2 yaira2 added the ready for review Pull requests that are ready for review label May 22, 2024
@yaira2 yaira2 requested a review from hishitetsu May 22, 2024 16:27
private static string NormalizePathInput(string currentInput, bool isFtp)
{
// Check if input is a command and not a path
if (currentInput.Length > 1 && currentInput.ElementAt(1) != ':')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is : always the second character in a valid path?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't the paths be relative? ./Child ../Sibling etc? It won't have a drive letter then

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not concerned since we don't have plans on supporting that, but worst case we can come up with a new fix if things change.

@FieryRMS
Copy link
Contributor

I think it will be a big hassle to individually blacklist/whitelist each case. My approach would be to pass the original currentInput to LaunchApplicationFromPath instead of the normalized one.

if (await LaunchApplicationFromPath(currentInput, workingDir))

Then after finding the filename, before LaunchAppAsync (line 827) we can NormalizePathInput if we require.

if (trimmedInput.Contains(' '))
{
var positionOfBlank = trimmedInput.IndexOf(' ');
fileName = trimmedInput.Substring(0, positionOfBlank);
arguments = currentInput.Substring(currentInput.IndexOf(' '));
}
return await LaunchHelper.LaunchAppAsync(fileName, arguments, workingDir);

@FieryRMS
Copy link
Contributor

FieryRMS commented May 22, 2024

if (trimmedInput.Contains(' '))
{
var positionOfBlank = trimmedInput.IndexOf(' ');
fileName = trimmedInput.Substring(0, positionOfBlank);
arguments = currentInput.Substring(currentInput.IndexOf(' '));
}
return await LaunchHelper.LaunchAppAsync(fileName, arguments, workingDir);

After looking a bit more into this part of the code and doing some testing, it seems like if there is spaces in the path of the application (and there is parameters to the program), the program fails to run
eg: C:\Users\FieryRMS\Desktop\Folder 2\test 1.exe param

I propose to additionally check if currentInput starts with "/', then it should get the fileName until the next "/' (or end if it doesn't exist). Then it should work if the command is "C:\Users\FieryRMS\Desktop\Folder 2\test 1.exe" param

@yaira2
Copy link
Member Author

yaira2 commented May 22, 2024

I think it will be a big hassle to individually blacklist/whitelist each case. My approach would be to pass the original currentInput to LaunchApplicationFromPath instead of the normalized one.

That's a good approach. Should I disregard #15448 (comment)?

@yaira2 yaira2 requested a review from FieryRMS May 22, 2024 20:13
Copy link
Contributor

@FieryRMS FieryRMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. built and tested it

@FieryRMS
Copy link
Contributor

Should I disregard #15448 (comment)?

That bug I mentioned should persist even after the current fix. This issue comes from
fileName = trimmedInput.Substring(0, positionOfBlank);. If file path is C:/Folder 1/test 1.exe param then fileName="C:/Folder" instead of "C:/Folder 1/test 1.exe". That's why I proposed that we can check for quotes to calculate the delimiting position, so if the user inputs "C:/Folder 1/test 1.exe" param we are able to correctly execute it.

I suppose I should make a separate issue for this. It seems unrelated to the issue at hand.

@yaira2
Copy link
Member Author

yaira2 commented May 22, 2024

Thanks!

@yaira2 yaira2 merged commit 8611491 into main May 23, 2024
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed ready for review Pull requests that are ready for review labels May 23, 2024
@yaira2 yaira2 deleted the ya/FixCommands branch May 26, 2024 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge Pull requests that are approved and ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Address bar formatting causes issues when running commands through address bar

4 participants